Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support running with TrustedTypes enforced #4447

Closed
wants to merge 3 commits into from

Conversation

rictic
Copy link

@rictic rictic commented Sep 15, 2020

Description of the Change

Trusted Types is a new Content Security Policy specification,
currently implemented in browsers based on Chromium 83 or higher, which
requires that data passed to APIs which may result in arbitrary code
execution must go through an explicit policy. This helps to catch
unintended use of dangerous APIs, and reduces the surface area for
some security reviews.

I'm not sure if test infrastructure like mocha is a likely target
for attack – seems like in most cases an attacker could only access test
data, and it is rare for tests to handle untrusted data. However,
there's value for infrastructure to be compatible with running with
Trusted Types enabled, as it will allow users to write tests to ensure
that the code under test can run with Trusted Types.

This change creates and applies policies for the two places in mocha
that call innerHTML, and adds a temporary patch to the rollup build.
With those changes in place, we can run mocha's karma tests with
Trusted Types enabled (save for the one test that runs with requirejs,
which relies on eval).

Alternate Designs

It would be possible to integrate dompurify into the HTML reporter, which would make it more secure. Alternatively, it may be possible to do a larger refactoring to construct the HTML output in a more structured way, rather than using string concatenation of HTML.

Why should this be in core?

This must be in core in order for these parts of core to be trusted types compatible.

Benefits

Users will be able to test their code with mocha in a browser with native Trusted Types enforcement enabled.

Possible Drawbacks

This change adds additional code, which adds weight to the mocha package for users, and maintenance burden on the mocha core devs.

Our own dependencies may inadvertently break us by doing otherwise safe operations (e.g. eval(str), Function(str), setting innerHTML, etc).

This is an enhancement (minor release).

More info and related work

Trusted Types is a new Content Security Policy specification,
currently implemented in browsers based on Chromium 83 or higher, which
requires that data passed to APIs which may result in arbitrary code
execution must go through an explicit policy. This helps to catch
unintended use of dangerous APIs, and reduces the surface area for
some security reviews.

I'm not sure if test infrastructure like mocha is a likely target
for attack – seems like in most cases an attacker could only access test
data, and it is rare for tests to handle untrusted data. However,
there's value for infrastructure to be compatible with running with
Trusted Types enabled, as it will allow users to write tests to ensure
that the code under test can run with Trusted Types.

This change creates and applies policies for the two places in mocha
that call innerHTML, and adds a temporary patch to the rollup build.
With those changes in place, we can run mocha's karma tests with
Trusted Types enabled (save for the one test that runs with requirejs,
which relies on eval).

More info:

* Spec: https://w3c.github.io/webappsec-trusted-types/dist/spec/#introduction
* Related PR adding support to karma: karma-runner/karma#3360
@jsf-clabot
Copy link

jsf-clabot commented Sep 15, 2020

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Sep 15, 2020

Coverage Status

Coverage increased (+0.007%) to 93.966% when pulling 095bc91 on rictic:trustedTypes into 738a575 on mochajs:master.

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Seems like a good addition. Unfortunately, I don't know anything about Trusted Types, so I think I need to educate myself or defer the decision to another maintainer who does (e.g., @Munter?).

My main question is: if a test is written using eval(), and we ship this change, will the test now fail when run in a Trusted-Types-supporting browser? If so, this behavior must be opt-in. And if it needs to be opt-in, does this defeat the purpose?

* 2021-01-01. This behavior is tested, so we can just remove the plugin
* from our array and try `npm test`. If the tests pass, this can be removed.
*/
const applyTemporaryCspPatchPlugin = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like others would have this problem. Is there not an "official" fix anywhere?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The latest version of regenerator-runtime is compatible with trusted types. It looks like some dependency of a dependency is shipping with an older regenerator-runtime version compiled in. That's why I'm expecting that in a few months they'll have shipped an update with the new regenerator-runtime and we can drop applyTemporaryCspPatchPlugin

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking from the perspective of a reviewer in September 2020: I'd want a comment in the code pointing to the relevant tracking issue. That way it's easier to get context on it and check whether it's been resolved.

Speaking from the perspective of a reviewer now, in 2024: I'm betting this has since been resolved. 😄

lib/reporters/html.js Outdated Show resolved Hide resolved
@boneskull boneskull added area: browser browser-specific type: feature enhancement proposal semver-minor implementation requires increase of "minor" version number; "features" labels Sep 25, 2020
@boneskull
Copy link
Contributor

Further, I think it'd be good to see a test which asserts the behavior works as intended.

@rictic
Copy link
Author

rictic commented Sep 26, 2020

My main question is: if a test is written using eval(), and we ship this change, will the test now fail when run in a Trusted-Types-supporting browser? If so, this behavior must be opt-in. And if it needs to be opt-in, does this defeat the purpose?

Trusted type enforcement is opt in. Like other CSP features, you enable it with either an HTTP header or with a <meta> tag in the page's <head>.

The change should have no effect on existing tests, which won't have opted into enforcing Trusted Types.

Shipping this change will make mocha compatible with trusted types, so it will enable testing with mocha with TT enforced. See e.g. lit/lit#1323.

So tests that use potential unsafe APIs like eval() will not be broken by this change. They'd need to take the additional step of configuring their test runtime to enforce trusted types, in which case breaking a naive use of eval() would be the point.

Further, I think it'd be good to see a test which asserts the behavior works as intended.

See the changes to karma.conf.js, which adds headers to enable Trusted Type enforcement for mocha's existing browser tests (in supporting browsers, which is currently browsers based on a recent Chromium like Brave, Chrome, and Edge).

@boneskull
Copy link
Contributor

Further, I think it'd be good to see a test which asserts the behavior works as intended.
See the changes to karma.conf.js, which adds headers to enable Trusted Type enforcement for mocha's existing browser tests (in supporting browsers, which is currently browsers based on a recent Chromium like Brave, Chrome, and Edge).

sorry, what I meant was that the existing tests assert compatibility, but we do not have:

  • a test asserting some sort of exception is thrown when unsafe code is run
  • a test asserting no such exception is thrown when unsafe code is run

@boneskull
Copy link
Contributor

(AppVeyor is no longer building our PRs; I've removed it from the list of checks)

@rictic
Copy link
Author

rictic commented Sep 30, 2020

I'm not 100% sure I follow. You'd like to see a test of native Trusted Types functionality? A test that only runs in Chromium browsers and would fail if the trusted types headers in karma.conf.js didn't result in TT being enforced?

@boneskull
Copy link
Contributor

I'm not 100% sure I follow. You'd like to see a test of native Trusted Types functionality? A test that only runs in Chromium browsers and would fail if the trusted types headers in karma.conf.js didn't result in TT being enforced?

Yes, essentially:

  • a test that runs in Chromium that asserts "unsafe" code will break
  • a test that runs either in Chromium w/ the headers disabled, or somewhere else that lacks support, and asserts "unsafe" code does not break

The point is not to test the browser feature--I'm sure Chromium has plenty of tests--but rather to establish a baseline of behavior which Mocha will exhibit when encountering these configurations. We need to make sure that future changes in Mocha do not violate the assumptions of its behavior established here.

@outsideris
Copy link
Contributor

outsideris commented Oct 3, 2020

I need to learn about TrustedTypes.

@boneskull
Copy link
Contributor

@rictic are you able to continue on this PR?

Copy link
Member

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good start & most of the way there! Requesting changes on the HTML reporter, with the note that I'm not confident. 🙂

Also, +1 to boneskull's requests for more testing.

* 2021-01-01. This behavior is tested, so we can just remove the plugin
* from our array and try `npm test`. If the tests pass, this can be removed.
*/
const applyTemporaryCspPatchPlugin = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Speaking from the perspective of a reviewer in September 2020: I'd want a comment in the code pointing to the relevant tracking issue. That way it's easier to get context on it and check whether it's been resolved.

Speaking from the perspective of a reviewer now, in 2024: I'm betting this has since been resolved. 😄

Comment on lines +29 to +32
createHTML: function(html) {
// The highlight function escapes its input.
return highlight(html);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Style] Nit: no need for this wrapper, I think?

Suggested change
createHTML: function(html) {
// The highlight function escapes its input.
return highlight(html);
}
// The highlight function escapes its input.
createHTML: highlight,

* processed by a secure sanitization system like dompurify
*/
return html;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Bug] Indeed, this looks like it partially (not completely) bypasses the point of Trusted Types. Which worries me. If Mocha were to ship with this change as-is and claim to supported Trusted Types, that claim would be a little misleading.

I'd think making this truly "safe" should be a blocker to the PR. But I'm very open to being convinced otherwise if I'm off.

@JoshuaKGoldberg JoshuaKGoldberg added the status: waiting for author waiting on response from OP - more information needed label Mar 1, 2024
@JoshuaKGoldberg
Copy link
Member

👋 coming back to this @rictic, are you still interested in working on this PR? As of #5027 there are again maintainers who can review it now. No worries if you no longer have time - but if you do that'd be great!

@JoshuaKGoldberg JoshuaKGoldberg changed the title Support running with TrustedTypes enforced. feat: support running with TrustedTypes enforced Mar 4, 2024
@JoshuaKGoldberg JoshuaKGoldberg added the stale this has been inactive for a while... label Jul 2, 2024
@justinfagnani
Copy link

I would still like to see this resolved, especially as Webkit has decided to support the standard as of two weeks ago: WebKit/standards-positions#186 (comment) and Mozilla as of Dec 2023: mozilla/standards-positions#20 (comment)

@JoshuaKGoldberg
Copy link
Member

Great, I look forward to your PR @justinfagnani. 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: browser browser-specific semver-minor implementation requires increase of "minor" version number; "features" stale this has been inactive for a while... status: waiting for author waiting on response from OP - more information needed type: feature enhancement proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants